feat: Add optimizely attribute fetching functionality#20
feat: Add optimizely attribute fetching functionality#20alexs-mparticle merged 7 commits intodevelopmentfrom
Conversation
invoke right before selectPlacement() calls
patrick-wu
force-pushed
the
feat/EXP-4851
branch
from
09130b2 to
4d7c571
Compare
There was a problem hiding this comment.
Copilot reviewed 1 out of 1 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (2)
src/Rokt-Kit.js:209
- Review the condition using a logical OR; verify that using || is intentional and that the behavior when only window.optimizely is truthy meets the intended logic.
if (forwarders.length > 0 || window.optimizely) {
src/Rokt-Kit.js:224
- Add a safeguard to ensure that optimizelyState.getVariationMap()[expId] is defined before accessing its .id property to prevent potential runtime errors.
acc['rokt.custom.optimizely.experiment.' + expId + '.variationId'] = optimizelyState.getVariationMap()[expId].id;
alexs-mparticle
left a comment
alexs-mparticle
left a comment
There was a problem hiding this comment.
Great work @patrick-wu! Appreciate you rebasing and resubmitting. So far I only have some minor nits but his looks good.
src/Rokt-Kit.js
Outdated
| }); | ||
|
|
||
| try { | ||
| if (forwarders.length > 0 || window.optimizely) { |
There was a problem hiding this comment.
I think logical AND would be more appropriate, right?
| if (forwarders.length > 0 || window.optimizely) { | |
| if (forwarders.length > 0 && window.optimizely) { |
There was a problem hiding this comment.
but what about the case where the optimizely sdk wasn't loaded via the kit setup, since the client can still load it directly?
mparticle doc on optimizely integration
While allowing mParticle to automatic load Optimizely reduces the amount of code you need to write, you may choose to initialize Optimizely yourself to prevent “page flashing” in the case where an Optimizely experiment is expected to alter the UI immediately on load. You can read more about this concern here and make the choice that’s best for your setup.
i guess even if they load it themselves, we'd still have a forwarder?
There was a problem hiding this comment.
forwarder would still exist yes.
There was a problem hiding this comment.
ok got it. in that case yeah, i'll modify this to &&
| try { | ||
| if (forwarders.length > 0 || window.optimizely) { | ||
| // Get the state object | ||
| var optimizelyState = window.optimizely.get('state'); |
There was a problem hiding this comment.
I think you should add some handlers in case optimizelyState returns undefined.
| this.selectPlacements = selectPlacements; | ||
|
|
||
| // mParticle Kit Callback Methods | ||
| function fetchOptimizely() { |
There was a problem hiding this comment.
Now that we have a testing framework, I would like to see some tests around this logic. I can help you with mocking if you need it.
There was a problem hiding this comment.
@alexs-mparticle can you take a look if these test cases are sufficient?
dist/Rokt-Kit.common.js
Outdated
| var selectPlacementsAttributes = mergeObjects(filteredAttributes, { | ||
| mpid: mpid, | ||
| }); | ||
| var optimizelyAttributes = |
There was a problem hiding this comment.
Please remove the dist files from the commit. These are built by the CI/CD process.
test/src/tests.js
Outdated
| getVariationMap: function () { | ||
| return {}; | ||
| }, | ||
| // Missing getActiveExperimentIds |
There was a problem hiding this comment.
Nitpick: Updating comment for clarity
| // Missing getActiveExperimentIds | |
| // Mocking a scenario for when getActiveExperimentIds() method is missing |
patrick-wu
force-pushed
the
feat/EXP-4851
branch
from
bba582e to
dd518f9
Compare
alexs-mparticle
changed the title
# [1.2.0](v1.1.0...v1.2.0) (2025-04-08) ### Features * Add optimizely attribute fetching functionality ([#20](#20)) ([fdf9ca9](fdf9ca9))
3 participants
Add Optimizely attribute-fetching function to Kit, invoke right before selectPlacement() calls
Summary
{provide a thorough description of the changes}
Testing Plan
{explain how this has been tested, and what additional testing should be done}